NETOBSERV-2977: Add TLS support for collector when OpenShift - #552
leandroberetta wants to merge 9 commits into
Conversation
|
@leandroberetta: This pull request references NETOBSERV-2515 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target either version "5.0.0." or "openshift-5.0.0.", but it targets "netobserv-2.0" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| sigs.k8s.io/yaml v1.6.0 // indirect | ||
| ) | ||
|
|
||
| replace github.com/netobserv/flowlogs-pipeline => github.com/leandroberetta/flowlogs-pipeline v0.0.0-20260810170916-6c5c94ab0294 |
There was a problem hiding this comment.
Don't forget to remove this :)
There was a problem hiding this comment.
that deserve at least a unit test
| if [[ "$tlsEnabled" == "true" ]]; then | ||
| cmd="${K8S_CLI_BIN} run -n $namespace collector \\ | ||
| --image=$img --image-pull-policy='Always' --restart='Never' \\ | ||
| --override-type=strategic \\ | ||
| --overrides=$overrides \\ | ||
| --command -- $runCommand" | ||
| else | ||
| cmd="${K8S_CLI_BIN} run -n $namespace collector \\ | ||
| --image=$img --image-pull-policy='Always' --restart='Never' \\ | ||
| --overrides=$overrides \\ | ||
| --command -- $runCommand" | ||
| fi |
There was a problem hiding this comment.
| if [[ "$tlsEnabled" == "true" ]]; then | |
| cmd="${K8S_CLI_BIN} run -n $namespace collector \\ | |
| --image=$img --image-pull-policy='Always' --restart='Never' \\ | |
| --override-type=strategic \\ | |
| --overrides=$overrides \\ | |
| --command -- $runCommand" | |
| else | |
| cmd="${K8S_CLI_BIN} run -n $namespace collector \\ | |
| --image=$img --image-pull-policy='Always' --restart='Never' \\ | |
| --overrides=$overrides \\ | |
| --command -- $runCommand" | |
| fi | |
| overrideType="" | |
| if [[ "$tlsEnabled" == "true" ]]; then | |
| overrideType="--override-type=strategic" | |
| fi | |
| cmd="${K8S_CLI_BIN} run -n $namespace collector \ | |
| --image=$img --image-pull-policy='Always' --restart='Never' \ | |
| $overrideType --overrides=$overrides \ | |
| --command -- $runCommand" |
There was a problem hiding this comment.
We should simplify this to something like:
# Create collector service for flows/packets captures
if [[ "$command" = "flows" || "$command" = "packets" ]]; then
echo "creating collector service"
applyYAML "$collectorServiceYAML"
if [[ "$tlsEnabled" == "true" ]]; then
echo "creating CA configmap for TLS"
createCAConfigMap
fi
fi
if [ "$command" = "flows" ]; then
echo "creating flow-capture agents"
elif [ "$command" = "packets" ]; then
echo "creating packet-capture agents"
elif [ "$command" = "metrics" ]; then
echo "creating service monitor"
applyYAML "$smYAML"
echo "creating metric-capture agents:"
| function isOpenShift() { | ||
| ${K8S_CLI_BIN} get clusterversion version &>/dev/null | ||
| } |
There was a problem hiding this comment.
You should rely on checkClusterVersion here instead.
Feel free to add a global variable like isOCP in it for your usage 😉
| golang.org/x/tools v0.45.0 // indirect | ||
| google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa // indirect | ||
| google.golang.org/grpc v1.81.1 // indirect | ||
| google.golang.org/grpc v1.82.0 |
There was a problem hiding this comment.
It's a direct dependency now: collector_tls.go uses grpc.Creds(credentials.NewTLS(...)) to enable TLS on the collector, so grpc is imported directly rather than transitively.
f86c30c to
54b1101
Compare
|
@jpinsonneau I addressed the feedback, the only missing one is the dependency update, I'm waiting to merge this: netobserv/flowlogs-pipeline#1297 |
47414b5 to
565fb2e
Compare
|
@leandroberetta: This pull request references NETOBSERV-2515 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target either version "5.1.0." or "openshift-5.1.0.", but it targets "netobserv-2.0" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
fd8151c to
8956222
Compare
leandroberetta
changed the title
|
@leandroberetta: This pull request references NETOBSERV-2977 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.1.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
90e2ebe to
dce2750
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #552 +/- ##
==========================================
+ Coverage 13.18% 17.31% +4.13%
==========================================
Files 20 24 +4
Lines 2443 2656 +213
==========================================
+ Hits 322 460 +138
- Misses 2095 2161 +66
- Partials 26 35 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
jpinsonneau
left a comment
jpinsonneau
left a comment
There was a problem hiding this comment.
LGTM ! Thanks @leandroberetta
|
/ok-to-test |
The gRPC connection between the capture agent (FLP client) and the CLI collector (gRPC server) hardcoded MinVersion: TLS 1.3 on both ends. Every other netobserv component derives its TLS settings from the OpenShift tlsSecurityProfile (apiservers.config.openshift.io/cluster); the CLI now does the same instead of pinning a version. A new `resolve-tls` subcommand runs as an initContainer on the collector pod. It reads the cluster's tlsSecurityProfile, resolves it to concrete min version / cipher suites / curves (falling back to the Intermediate preset when no profile is set, mirroring the operator's default), and writes them into the `collector-tls-config` ConfigMap. Both the collector container and the agent DaemonSet consume that ConfigMap via envFrom, so a single resolver run drives both ends. The collector server applies the resolved settings through flowlogs-pipeline's tlsprofile.Apply. Because the ConfigMap only exists once the collector's init completes, the install order flips on the TLS path: the collector is created and waited on first, then the agents (which reference the ConfigMap with optional:false, failing loudly rather than silently downgrading TLS). This path stays OpenShift-only and applies to flows/packets captures; metrics, --yaml output and non-OCP runs are unchanged. - internal/pkg/tlsresolver: profile resolution + ConfigMap write, with tests - cmd/resolve_tls.go, cmd/root.go: new resolve-tls subcommand - cmd/collector_tls.go: drop hardcoded TLS 1.3, apply resolved profile - res/service-account.yml: RBAC to read apiservers/cluster and write the CM - commands/netobserv, scripts/functions.sh: initContainer, envFrom, ordering - go.mod: add openshift/api and openshift/library-go/pkg/crypto Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The TLS profile resolver maps the OpenShift SecP256r1MLKEM768 / SecP384r1MLKEM1024 groups to their crypto/tls.CurveID constants, which are Go 1.26+. CI (setup-go 1.26), the Dockerfile builder (golang:1.26) and the operator (go 1.26.3) are already on 1.26; only the go.mod directive lagged at 1.25.7, which made govet's stdversion reject those constants and the exhaustive linter reject dropping them. Bump the directive to 1.26.0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
agentManifest is set in functions.sh setup() but consumed in commands/netobserv (applied after the collector is ready). shellcheck analyzes each file in isolation, so it flags the assignment as unused; add the same disable directive the file already uses elsewhere. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Moving the resolve-tls initContainer's namespaced Role and RoleBinding to the end of res/service-account.yml keeps the existing service-account document indices stable, but still adds two documents to every generated capture manifest. Update the positional assertions in the flow, packet and metric YAML e2e tests accordingly: - bump the expected document counts (8 -> 10 for flows/packets, 12 -> 14 for metrics), - assert the new Role (configmaps get/create/update) at index [6] and RoleBinding (ServiceAccount -> Role netobserv-cli) at index [7], - assert the new config.openshift.io/apiservers get rule on the netobserv-cli ClusterRole, - shift the collector Service / DaemonSet and the metric-specific documents to their new indices. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
go.mod requires go >= 1.26.0 (the TLS profile resolver uses the crypto/tls SecP256r1MLKEM768 / SecP384r1MLKEM1024 CurveID constants, added in Go 1.26), but the build root was still rhel-9-release-golang-1.25-openshift-4.21, which ships Go 1.25.12 with GOTOOLCHAIN=local. The netobserv-cli-tests step therefore failed immediately on `go list -m github.com/onsi/ginkgo/v2` with "go.mod requires go >= 1.26.0 (running go 1.25.12; GOTOOLCHAIN=local)". Move to rhel-9-release-golang-1.26-openshift-5.0, the build root already used by network-observability-operator, flowlogs-pipeline and netobserv-ebpf-agent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
StartCommand called pty.Start inside a goroutine and only did `_ = ptmx`, which does not extend the file's lifetime past that goroutine. The runtime finalizer then closed the PTY master, the kernel sent SIGHUP to the foreground process group, and the CLI died about 20s in -- running its EXIT trap, which deletes the daemonset, the collector pod and the namespace. `--max-time` was therefore never honoured in e2e; the whole suite really had a ~20s budget, and StartCommandWait was bumped to 20s to snapshot the output just before the process was killed. That was survivable while the daemonset was created in setup(), a second into the run. With the collector TLS path it is created only once the collector is ready (the agents consume the collector-tls-config ConfigMap written by the resolve-tls initContainer), i.e. about 8s in, leaving ~13s before the SIGHUP -- not enough for the agent pods to report ready, so "Verify all CLI pods are deployed" polled a daemonset that no longer existed until it timed out: 17:33:59.4 create daemonsets/netobserv-cli 17:34:12.4 delete daemonsets/netobserv-cli <- CLI EXIT trap after SIGHUP 17:34:16 test starts seeing NotFound, for 10 minutes Hold the PTY master in a package-level slice and drain it, so the command runs until its own --max-time. Since captures now outlive the spec, add StopStartedCommands and call it from cleanup(): otherwise a lingering CLI would fire its EXIT trap while the next spec is setting up and delete that spec's namespace. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
a2b51bb to
3205dfa
Compare
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@leandroberetta: This pull request references NETOBSERV-2977 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "5.1.0." or "openshift-5.1.0.", but it targets "netobserv-2.0" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@leandroberetta: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/label qe-approve Tested and its behavior depend upon the when the node reconcile after policy change |
|
@kapjain-rh: The label(s) `/label qe-approve
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
5 participants
Description
Enable TLS for the collector↔agent gRPC connection when running on OpenShift, and make that connection honor the cluster's TLS security profile.
TLS enablement (service-ca)
Honor the cluster TLS security profile
How to test
Scope
This change only affects
oc netobserv flowsandoc netobserv packetsrunning against an OpenShift cluster. Everything else is expected to behave exactly as onmain:oc netobserv metrics— unchanged (no collector pod involved)--yamloutput — unchanged (no TLS resources are added to the generated manifests)Prerequisites
An OpenShift cluster with the service-ca operator running (standard on OCP)
cluster-admin, since the capture creates a namespace, SCC and a ClusterRoleThe CLI build from this PR. CI posts a comment with the image and the exact
make commandsline to run once the PR carries theok-to-testlabel. If you build locally instead:All captures below run in the
netobserv-clinamespace by default (override withNETOBSERV_NAMESPACE).Terminology used below
The resolver writes the cluster's TLS profile into a ConfigMap as decimal values. For
TLS_MIN_VERSION:Scenario 1 — Default cluster (no explicit tlsSecurityProfile)
A stock cluster has no
spec.tlsSecurityProfileset on the APIServer, and the CLI must fall back to the Intermediate preset (the same default the operator uses).Confirm the cluster has no explicit profile:
Start a capture and leave it running:
Expected CLI output — these lines must appear, in this order:
Note the ordering: on the TLS path the collector is created first, and the agents only after the collector pod is Ready. This is intentional — the agents mount a ConfigMap the collector's initContainer produces.
From a second terminal, check the resources:
Expected:
collector-tls-configexists and containsTLS_MIN_VERSION: "771"(TLS 1.2 = Intermediate), plus non-emptyTLS_CIPHER_SUITESandTLS_CURVE_PREFERENCES. Theresolve-tlslog should say it resolved profileIntermediate.Check both ends picked it up:
Expected:
caCertPath: /etc/collector-ca/service-ca.crtin the FLP config, andconfigMapRef: collector-tls-configwithoptional: falseon both the collector and the DaemonSet.Most important check — flows still arrive. The capture table must populate with flows and the agent logs must be free of TLS handshake errors:
Expected: no errors. A working capture here is the real proof the whole chain (cert generation → CA injection → profile resolution → mutual agreement on cipher/version) lines up.
Let the capture finish (or Ctrl-C), answer the copy prompt, and confirm the output files are written as usual.
Scenario 2 — Packets capture
Repeat scenario 1 with:
Expected: identical TLS behavior (this path previously did not even create the CA ConfigMap), and packets captured normally.
Scenario 3 — Explicit
ModernprofileThis is the check that the connection genuinely follows the cluster profile rather than a hardcoded value.
Then run a capture and inspect the ConfigMap:
Expected:
TLS_MIN_VERSION: "772"(TLS 1.3), and the capture works end to end.Optional on-the-wire confirmation — a TLS 1.2 client must now be rejected:
oc -n netobserv-cli run tlscheck --rm -i --restart=Never \ --image=registry.access.redhat.com/ubi9/ubi -- \ openssl s_client -connect collector.netobserv-cli.svc:9999 -tls1_2 </dev/nullExpected with Modern: handshake failure (protocol version alert).
Expected with Intermediate: handshake succeeds and reports
Protocol : TLSv1.2.Scenario 4 — Explicit
OldprofileExpected:
TLS_MIN_VERSION: "769"(TLS 1.0) incollector-tls-config, and the capture still works. The point here is that the CLI does not refuse or silently upgrade a permissive cluster profile.Scenario 5 —
CustomprofileExpected:
TLS_MIN_VERSION: "771"and aTLS_CIPHER_SUITESlist restricted to the two suites requested (as decimal IDs). Capture works.Remember to restore the cluster afterwards:
oc patch apiserver cluster --type=json -p '[{"op":"remove","path":"/spec/tlsSecurityProfile"}]'Scenario 6 — Regression: paths that must be untouched
oc netobserv metrics --max-time=5mresolve-tlsinitContainer, nocollector-tls*resources, metrics dashboard works as onmainoc netobserv flows --yamlcapture.ymlcontains no TLS volumes, initContainer or ConfigMap refs; applying it still worksflowsagainst a kind / vanilla k8s clusterCan't check version since cluster is not OpenShift, plaintext capture, no TLS resources created, flows arriveoc netobserv flows --backgroundthenoc netobserv follow/stop/copymain, with TLS transparently enabledoc get ns netobserv-clireturns NotFound — the namespace deletion takes the Role, RoleBinding, Secret and both ConfigMaps with itScenario 7 — Fail-loud behavior (negative test)
The agents reference
collector-tls-configwithoptional: falseon purpose: a missing ConfigMap must make the agent pods fail visibly rather than silently fall back to plaintext.To simulate, in one terminal start a capture, and as soon as the agents come up delete the ConfigMap and restart the DaemonSet:
Expected: agent pods stuck in
CreateContainerConfigErrorwith an event naming the missing ConfigMap. They must not start and send flows in plaintext.New RBAC to sanity-check
The capture now grants itself two extra permissions. Confirm they are present and no broader than described:
Expected:
getonapiserverslimited viaresourceNames: [cluster], andget/create/updateonconfigmapsscoped to the run namespace (nodelete, no cluster-wide ConfigMap access).Dependencies
n/a
Checklist